Button in menu #256
Button in menu #256
Conversation
|
nice! yeh this declarative API seems to be consistent with how we do everything else. Can you add a button to the Redux example that fires some redux action? |
bpostlethwaite
left a comment
bpostlethwaite
left a comment
There was a problem hiding this comment.
I like the declarative API! It fits with the rest of the App. A few suggestions have been made.
src/components/PanelMenuWrapper.js
Outdated
| groupIndex = groupLookup[group]; | ||
| obj = groups[groupIndex]; | ||
| } else { | ||
| if (child.type.name === 'Button') { |
There was a problem hiding this comment.
Are we expecting this always to be our Button type? If so we can compare classes instead of child.type.name. From Section.js
if (child.type === Info) {
...
}
Do we need to restrict users to our Button? Is there a way to allow users to pass any arbitrary component to be added to the Sidebar.
What if we had a SidebarItem component that wraps whatever you want to include in the sidebar?
<PanelMenuWrapper >
<SidebarItem name="save">
<a onClick={saveMyApp} />
</SidebarItem>
</PanelMenuWrapper>This way users can put any thing in a SidebarItem container and we just render it.
src/components/PanelMenuWrapper.js
Outdated
| } else { | ||
| if (child.type.name === 'Button') { | ||
| if (groupLookup.hasOwnProperty(group)) { | ||
| throw new Error('cannot have 2 menu buttons with same name'); |
There was a problem hiding this comment.
Is there a solid reason why we can't have 2 buttons with the same name?
src/components/widgets/Button.js
Outdated
|
|
||
| classes += ` ${className}`; | ||
| if (group) { | ||
| classes += ' button--menu'; |
There was a problem hiding this comment.
In the name of keeping our widgets dumb perhaps instead of adding logic that knows about groups in a the button widget maybe we should just pass in a className. So th parent would supply button--menu and the child would simply render whatever class it is passed.
| obj = groups[groupIndex]; | ||
| } else { | ||
| groupLookup[group] = groups.length; | ||
| obj = {name: group, panels: []}; |
There was a problem hiding this comment.
Could obj be {name: group} with no panels? Then later on we would handle the case where panels is undefined?
not a huge deal but having to create empty arrays seems to suggest we generalize the framework just a bit.
src/styles/variables/_theme.scss
Outdated
| ); | ||
| // Buttons | ||
| $buttons: ( | ||
| menu:(padding-top: 10px), |
There was a problem hiding this comment.
hmm I would perhaps not do this, and instead use one of the spacing variables we have eg, padding-top: var(--spacing-quarter-unit); on the .button--menu class, so that we can keep all spacing units consistent
| } | ||
|
|
||
| &--isMenu { | ||
| @include button(menu); |
There was a problem hiding this comment.
This looks like it might not actually be doing anything. I don't see any edits to the mixin for this, @mixin button{ ... } found in the _mixins.scss partial.
|
ok, so looks like this now: |
…baritem to more appropriate export sections
|
alright, @bpostlethwaite @aulneau ready for another look! |
| @@ -0,0 +1,2 @@ | |||
| import Button from './Button'; | |||
There was a problem hiding this comment.
I hadn't added a widget index since widgets are meant to be internal. That said I am using some in the batch app so apparently they are not that internal. I suppose we may as expose more widgets!
There was a problem hiding this comment.
we could put Dropdown in here eventually as well (and other useful components folks may wish to use in their apps)
| </Fold> | ||
| </LayoutPanel> | ||
| <SingleSidebarItem> | ||
| <Button |
|
thanks for putting this in looks great 💃 ! |
to close: #255
@bpostlethwaite what would you think of something like this?
@aulneau if that principle's ok, would you help me with the styling of this...